Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameterize key query fields in riem_measures() #48

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

JElchison
Copy link

Expose key query fields in riem_measures() as parameters, including:

  • data (defaults to all as before)
  • elev (defaults to no as before)
  • latlon (defaults to yes as before)
  • report_type (new default is 3,4, which skips HFMETAR)

Update date_start to have safer default (2024 instead of 2014). Specify missing nometa query field. Update comments, self-documentation, and lintr exceptions accordingly.

@JElchison JElchison marked this pull request as draft December 16, 2024 20:18
@JElchison JElchison marked this pull request as ready for review December 16, 2024 20:38
Copy link
Member

@maelle maelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

R/measures.R Outdated Show resolved Hide resolved
R/measures.R Outdated Show resolved Hide resolved
R/measures.R Outdated Show resolved Hide resolved
R/measures.R Outdated Show resolved Hide resolved
R/measures.R Show resolved Hide resolved
R/measures.R Outdated Show resolved Hide resolved
@JElchison JElchison marked this pull request as draft December 17, 2024 15:29
@JElchison JElchison marked this pull request as ready for review December 17, 2024 16:17
@JElchison
Copy link
Author

Ready for review, pending my outstanding question on default value for latlon. Thanks!

@JElchison
Copy link
Author

I'm unsure what your standard practice is, but I'll leave it to you to click "Resolve conversation" on any items resolved to your liking. Thanks!

@maelle
Copy link
Member

maelle commented Dec 19, 2024

@JElchison Thank you!! I made a few changes

  • use of rlang::is_character() since the package is importing rlang anyway.
  • made report_type hopefully more readable.
  • added some tests.

I have two requests for you:

  • do you agree with these changes?
  • I was not able to find a test case for "report_type". Could you help? It also might be that my refactoring of report_type's handling inadvertently broke it.

Afterwards I'll update the NEWS file and will plan a new release in January. Thanks again!

R/measures.R Outdated Show resolved Hide resolved
NAMESPACE Outdated Show resolved Hide resolved
R/measures.R Show resolved Hide resolved
@JElchison
Copy link
Author

  • do you agree with these changes?

Yes, they look good! I asked a couple clarifying questions as comments.

  • I was not able to find a test case for "report_type". Could you help? It also might be that my refactoring of report_type's handling inadvertently broke it.

Yes, will do!

date_end = "2013-12-01"))
})

# TODO validate 'data' against [all, tmpf, dwpf, relh, drct, sknt, p01i, alti, mslp, vsby, gust, skyc1, skyc2, skyc3, skyc4, skyl1, skyl2, skyl3, skyl4, wxcodes, ice_accretion_1hr, ice_accretion_3hr, ice_accretion_6hr, peak_wind_gust, peak_wind_drct, peak_wind_time, feel, metar, snowdepth] # nolint: line_length_linter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we tackle this in this PR?

@maelle
Copy link
Member

maelle commented Dec 20, 2024

@JElchison Thank you! Are you happy with my own changes to the checks on the data arguments? I'll come back to this PR in the new year, merge it and work on a release (I might also tackle the other breaking change at the same time). Thank you and happy holidays!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants